Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated to support OpenSSL 3 #295

Merged
merged 11 commits into from
Feb 22, 2023
Merged

Updated to support OpenSSL 3 #295

merged 11 commits into from
Feb 22, 2023

Conversation

ctjhai
Copy link
Contributor

@ctjhai ctjhai commented Jan 19, 2023

This PR aims to address issue #259.

@ctjhai
Copy link
Contributor Author

ctjhai commented Jan 24, 2023

Hi @bifurcation, as a follow up of our discussion in #259, I've been looking at OpenSSL 3.x source code, and found that the flag EVP_MD_CTX_FLAG_NON_FIPS_ALLOW is not used anymore, see:
https://github.com/openssl/openssl/blob/3147785eb23bb27080a0b7accbbff46ac471e86c/include/openssl/evp.h#L208-L209

The discussion around this can be found here: openssl/openssl#17484.

I also found that in some OpenSSL versions (1.1.1) that I've tested, the flag EVP_MD_CTX_FLAG_NON_FIPS_ALLOW is only used to load non-FIPS implementation, e.g. loading MD5 implementation in FIPS mode; it doesn't really validate the constraint on the key size. I guess some custom implementation of the FIPS object module may be stricter, so perhaps this flag has been used to override this behaviour?

Anyway, with OpenSSL 3.x, perhaps we need to ensure that both FIPS and default providers are loaded, so that we can fallback to the default implementation to allow a more relaxed key size. So there is no need to force the use OpenSSL 1.1.1 for the FIPS mode. Having said that, just like version 1.1.1, it looks like OpenSSL 3.x is happy with HMAC key size < 112 bits even in FIPS mode. I tried 0-length HMAC key size with OpenSSL 3.0 FIPS provider, there is no issue; I arrive at the expected HMAC digest.

There are some test cases that fail in FIPS mode though, but I think this is related to using the likes of X25519 and X448 for key-agreement or Ed25519 in the X.509. I'll update the test cases as part of this pull-request.

Copy link
Contributor

@bifurcation bifurcation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great start, thanks @ctjhai. A few suggestions for refactoring, and it looks like you have some CI issues to clear, but overall this seems like the right direction.

Given the depth of the changes in group.cpp and digest.cpp, I wonder if it would be simpler just to have two versions of the files (one with the v1 implementation and one with v3), and switch between them in CMake. One of my concerns here is how easy it's going to be to revert this change once we're more in an all-v3 world, and that would certainly make that reversion simple.

It would also be good to add a CI run here that builds with OpenSSL v3. Might need an alternate vcpkg.json that gets swapped in at the right moment.

Comment on lines 202 to 203
, not_before(asn1_time_to_chrono(X509_getm_notBefore(x509.get())))
, not_after(asn1_time_to_chrono(X509_getm_notAfter(x509.get())))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, the getm versions return mutable references, which seems unnecessary here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, this should be X509_get0_XXX.

lib/hpke/src/digest.cpp Outdated Show resolved Hide resolved
lib/hpke/src/group.cpp Outdated Show resolved Hide resolved
lib/hpke/src/group.cpp Outdated Show resolved Hide resolved
}
}

auto bld = make_typed_unique(OSSL_PARAM_BLD_new());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might make this name a little more descriptive, say builder

lib/hpke/src/group.cpp Show resolved Hide resolved
@ctjhai
Copy link
Contributor Author

ctjhai commented Jan 25, 2023

Given the depth of the changes in group.cpp and digest.cpp, I wonder if it would be simpler just to have two versions of the files (one with the v1 implementation and one with v3), and switch between them in CMake. One of my concerns here is how easy it's going to be to revert this change once we're more in an all-v3 world, and that would certainly make that reversion simple.

The changes in digest.cpp are pretty small compared to group.cpp, so I am not entirely sure about creating a separately file for this. Additionally, another advantage of not creating two files is we can easily access the trails of changes of the files (from git blame perspective). In terms of reversion, we can use tools like unifdef to pick the code for OpenSSL 3 only.

Having said that, if you insist, I am happy to use two separate files for these. But we still need to cater for the WITH_OPENSSL3 flag in openssl_common.cpp. So would you also like to have two files for openssl_common.cpp to maintain consistency?

* Refactored group.cpp
* Added Github actions for OpenSSL 3
* Added OpenSSL vcpkg.json for MLS library and interop binary
@ctjhai ctjhai marked this pull request as ready for review January 27, 2023 23:41
@ctjhai ctjhai marked this pull request as draft February 1, 2023 09:14
@ctjhai ctjhai marked this pull request as ready for review February 1, 2023 13:07
Copy link
Contributor

@bifurcation bifurcation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ctjhai, this is on the right track. A couple of minor things, then this is good to go.

@@ -0,0 +1,43 @@
name: Build for older MacOS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to run this combination in CI. Support for older macOS is mainly a question of variant and optional, which is orthogonal to OpenSSL versions.

@@ -0,0 +1,42 @@
name: Build the interop harness (OpenSSL 3)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, the interop harness's issues are separate from any OpenSSL issues.

Comment on lines 118 to 126
auto mac =
make_typed_unique(EVP_MAC_fetch(nullptr, OSSL_MAC_NAME_HMAC, nullptr));
auto ctx = make_typed_unique(EVP_MAC_CTX_new(mac.get()));
auto digest_name = openssl_digest_name(id);
std::array<OSSL_PARAM, 2> params = {
OSSL_PARAM_construct_utf8_string(
OSSL_ALG_PARAM_DIGEST, digest_name.data(), 0),
OSSL_PARAM_construct_end()
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would switch the order of initialization here, so that if (ctx == nullptr) immediately follows the declaration of ctx. So:

auto digest_name = ...
auto params = std::array<OSSL_PARAM, 2>{ ... };
auto mac = ...
auto ctx = ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(const auto if possible)

@@ -0,0 +1,43 @@
name: Build for older MacOS (OpenSSL 3)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this extra build. The MacOS compat issues are orthogonal to OpenSSL 3.

CMakeLists.txt Outdated
# External libraries
find_package(OpenSSL REQUIRED)
if ( OPENSSL_FOUND )
if (${OPENSSL_VERSION} VERSION_GREATER 1.1.1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (${OPENSSL_VERSION} VERSION_GREATER 1.1.1)
if (${OPENSSL_VERSION} VERSION_GREATER_EQUAL 3)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This avoids spurious errors if the 1.X line evolves)

lib/hpke/src/digest.cpp Show resolved Hide resolved
Comment on lines 14 to 21
static int
FIPS_mode()
{
if (OSSL_PROVIDER_available(nullptr, "fips") == 1) {
return 1;
}
return EVP_default_properties_is_fips_enabled(nullptr);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than emulate the old OpenSSL FIPS API, I would pull the contents of fips.cpp/h into this file and its header, and make variants of fips() and enable_fips() for the two versions. Where in the latter case, I mean either make a new function enable_fips() that varies between the versions, or just branch internally to ensure_fips_if_required().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a problem

Comment on lines 3 to 7
bool
fips();

bool
is_fips_approved(mls::CipherSuite::ID id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These methods seem duplicative of fips and fips_disable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is no longer needed, so removed in the current commit.

test/fips.h Outdated
Comment on lines 3 to 7
bool
fips();

bool
is_fips_approved(mls::CipherSuite::ID id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we need these methods in test, let's just make them a public API of the hpke library. We shouldn't be directly including OpenSSL headers outside of HPKE.

Copy link
Contributor Author

@ctjhai ctjhai Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is no longer needed. Initially, I needed it when I tested my changes on an Oracle Linux 8 with FIPS enabled. The OS came with OpenSSL 1.1.1k FIPS and it had a smaller set of FIPS approved algorithms compared to OpenSSL 3.0 FIPS. So I had to use the above method to skip certain algorithm choices. Given that we are moving towards the later version of OpenSSL, I don't see there is a need to skip any test so I think we can remove this method.

Copy link
Contributor

@bifurcation bifurcation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the work on this @ctjhai !

@bifurcation
Copy link
Contributor

@ctjhai - Some CI runs are failing due to deficiencies in the macOS GitHub Actions runners. These were fixed in #314, but your PR is older than that. If you could please do a quick git checkout main && git pull && git checkout openssl3 && git merge main, that should turn the CI green and I'll merge.

@ctjhai
Copy link
Contributor Author

ctjhai commented Feb 22, 2023

@bifurcation Thanks Richard. Unfortunately, you will need to re-approve the CI run again. On my fork repo, all of the CI runs are all good now.

@bifurcation bifurcation merged commit ebc2d63 into cisco:main Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants